Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[10.x] Fix Http::retry so that throw is respected for call signature Http::retry([1,2], throw: false) #52002

Merged

Conversation

paulyoungnb
Copy link
Contributor

@paulyoungnb paulyoungnb commented Jul 3, 2024

Http::retry accepts the first parameter for times as an array|int. The documentation states that:

If all of the requests fail, an instance of Illuminate\Http\Client\RequestException will be thrown. If you would like to disable this behavior, you may provide a throw argument with a value of false.

However an Illuminate\Http\Client\RequestException is thrown as can be seen by adding a test to HttpClientTest:

public function testRequestExceptionIsNotThrownWhenDisabledAndRetriesExhaustedWithBackoffArray()
{
    $this->factory->fake([
        '*' => $this->factory->response(['error'], 403),
    ]);

    $response = $this->factory
        ->retry([1,2], throw: false)
        ->get('http://foo.com/get');

    $this->assertTrue($response->failed());

    $this->factory->assertSentCount(3);
}

Other tests have just been added to cover usage of the first parameter as an array.

This issue is due to PendingRequest.php:927 and PendingRequest.php:931 where the following comparisons are used:

if ($attempt < $this->tries && $shouldRetry) {
if ($this->tries > 1 && $this->retryThrow) {

even though $this->tries is an array. Suggested fix from this PR is to use:

$tryCount = is_array($this->tries) ? count($this->tries) + 1 : $this->tries;

in place of $this->tries. The count($this->tries) + 1 is required as specifying the retries in an array already assumes that the first request has been made and that the backoff times specified will be used. Whereas specifying the number of tries assumes that it includes the first request.

@taylorotwell taylorotwell merged commit fc70c5e into laravel:10.x Jul 3, 2024
24 checks passed
@driesvints driesvints changed the title Fix Http::retry so that throw is respected for call signature Http::retry([1,2], throw: false) [10.x] Fix Http::retry so that throw is respected for call signature Http::retry([1,2], throw: false) Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants